fix(cli): show Pi community provider in archon setup multiselect (#1526)#1
fix(cli): show Pi community provider in archon setup multiselect (#1526)#1
Conversation
…eam00#1526) - Replace hardcoded Claude/Codex options with dynamic provider list - Built-in providers shown first with SDK hints - Community providers shown with 'community' hint - Skip auth collection for non-built-in providers (Pi manages its own auth via ~/.pi/) - Set defaultAssistant correctly for community-only or mixed selections
There was a problem hiding this comment.
Code Review
This pull request refactors the AI assistant configuration in the setup command to dynamically support both built-in and community providers. Key changes include dynamic multiselect option generation and improved validation of tool availability. The review feedback highlights several areas for improvement: ensuring the default assistant logic only considers successfully configured providers, optimizing performance by reusing the registered providers list, and refining the selection priority to favor active built-in assistants over community ones.
| const builtinSelected = selectedProviders.filter(id => | ||
| getRegisteredProviders().find(p => p.id === id && p.builtIn) | ||
| ); |
There was a problem hiding this comment.
The builtinSelected list currently includes providers that the user might have opted to skip (e.g., if the binary was missing and they chose to continue without it). This can lead to a skipped provider being set as the default assistant or appearing in the selection prompt. It should be filtered based on finalHasClaude and finalHasCodex to reflect only available providers.
| const builtinSelected = selectedProviders.filter(id => | |
| getRegisteredProviders().find(p => p.id === id && p.builtIn) | |
| ); | |
| const activeBuiltins = []; | |
| if (finalHasClaude) activeBuiltins.push('claude'); | |
| if (finalHasCodex) activeBuiltins.push('codex'); |
| if (builtinSelected.length > 1) { | ||
| // Multiple built-in providers: prompt user to choose default | ||
| const providerChoices: { value: string; label: string }[] = []; | ||
| for (const id of builtinSelected) { | ||
| const p = getRegisteredProviders().find(p => p.id === id); | ||
| if (p) { |
There was a problem hiding this comment.
Use the filtered activeBuiltins list and the existing providers variable to ensure only available providers are considered for the default assistant selection.
let defaultAssistant: string;
if (activeBuiltins.length > 1) {
// Multiple built-in providers: prompt user to choose default
const providerChoices: { value: string; label: string }[] = [];
for (const id of activeBuiltins) {
const p = providers.find(p => p.id === id);
if (p) {| } else if (builtinSelected.length === 1) { | ||
| // Single built-in provider selected | ||
| defaultAssistant = builtinSelected[0]; | ||
| } else if (selectedProviders.length > 0) { | ||
| // Only community providers selected — use first one | ||
| defaultAssistant = selectedProviders[0]; | ||
| } else { | ||
| // Fallback to first available built-in | ||
| defaultAssistant = getRegisteredProviders().find(p => p.builtIn)?.id ?? 'claude'; | ||
| } |
There was a problem hiding this comment.
The logic for determining the defaultAssistant should prioritize active built-in providers and then fall back to selected community providers. The current implementation might pick a skipped built-in provider if it was the first one in selectedProviders. Using activeBuiltins and communitySelected ensures a valid configuration.
| } else if (builtinSelected.length === 1) { | |
| // Single built-in provider selected | |
| defaultAssistant = builtinSelected[0]; | |
| } else if (selectedProviders.length > 0) { | |
| // Only community providers selected — use first one | |
| defaultAssistant = selectedProviders[0]; | |
| } else { | |
| // Fallback to first available built-in | |
| defaultAssistant = getRegisteredProviders().find(p => p.builtIn)?.id ?? 'claude'; | |
| } | |
| } else if (activeBuiltins.length === 1) { | |
| // Single built-in provider selected | |
| defaultAssistant = activeBuiltins[0]; | |
| } else if (communitySelected.length > 0) { | |
| // Only community providers selected — use first one | |
| defaultAssistant = communitySelected[0]; | |
| } else { | |
| // Fallback to first available built-in | |
| defaultAssistant = providers.find(p => p.builtIn)?.id ?? 'claude'; | |
| } |
| const hasCommunityProvider = selectedProviders.some( | ||
| id => !getRegisteredProviders().find(p => p.id === id)?.builtIn | ||
| ); |
There was a problem hiding this comment.
The getRegisteredProviders() function is called repeatedly within the some loop, which is inefficient. Since the providers variable is already available from line 698, it should be reused. Additionally, pre-calculating the list of selected community providers here will simplify the defaultAssistant logic later.
| const hasCommunityProvider = selectedProviders.some( | |
| id => !getRegisteredProviders().find(p => p.id === id)?.builtIn | |
| ); | |
| const communitySelected = selectedProviders.filter(id => | |
| providers.find(p => p.id === id && !p.builtIn) | |
| ); | |
| const hasCommunityProvider = communitySelected.length > 0; |
Summary
Describe this PR in 2-5 bullets:
archon setupwizard hardcodes only Claude and Codex in the AI assistant multiselect. The Pi community provider (builtIn: false) is never shown, so users cannot select it as their default assistant during setup.collectAIConfig()now builds multiselect options dynamically fromgetRegisteredProviders(), supports community providers, and skips auth collection for non-built-in providers.SetupConfig[\"ai\"]interface, env generation logic, existing tests.UX Journey
Before
After
Architecture Diagram
Connection inventory
Label Snapshot
risk: lowsize: Sclicli:setupChange Metadata
featurecliLinked Issue
archon setupAI assistant selection coleam00/Archon#1526Validation Evidence (required)
Security Impact (required)
NoNoNoNoCompatibility / Migration
YesNoNoHuman Verification (required)
Side Effects / Blast Radius (required)
Rollback Plan (required)
git revert HEADRisks and Mitigations
None - this is a straightforward enhancement that uses existing registry infrastructure.